Conversation
| popUpTo(Screen.HomeScreen.route) { | ||
| inclusive = true | ||
| } |
There was a problem hiding this comment.
Just so I'm understanding correctly. This means that when I'm on the HomeScreen, and I navigate to signIn, the homeScreen is immediately popped off, right? Makes sense to me, but pop* always confuses me.
There was a problem hiding this comment.
Yes, if you navigate from home to signin, then home is popped off
| navigateToHome = { | ||
| navController.navigate(Screen.HomeScreen.route) { | ||
| popUpTo(Screen.SignInScreen.route) { | ||
| inclusive = true | ||
| } | ||
| } | ||
| }, | ||
| setLoggedIn = signInViewModel::signIn, | ||
| isSignedIn = signInViewModel.loggedInState, |
There was a problem hiding this comment.
I like this approach so that SignInScreen has no dependency on SignInViewModel
| DisposableEffect(key1 = isSignedIn, effect = { | ||
| if (isSignedIn) { | ||
| navigateToHome() | ||
| } | ||
|
|
||
| onDispose { | ||
| } | ||
| }) |
There was a problem hiding this comment.
I like this since I don't need an if statement for isSignedIn and instead basically declare the effect with it's key.
I am new to LaunchedEffect and DisposableEffect though so I will have to read up the differences on why you chose to replace LaunchedEffect with Disposable.
There was a problem hiding this comment.
I replaced it with DisposableEffect because you don't actually need the coroutine that is executed with LaunchedEffect.
| DisposableEffect(isSignedIn) { | ||
| if (!isSignedIn) { | ||
| navigateToSignIn() | ||
| } | ||
|
|
||
| onDispose { | ||
| } | ||
| } |
There was a problem hiding this comment.
Same as the above. I like that I can almost have a section of "effects" vs just having if statesments littered throughout.
There was a problem hiding this comment.
It's more accurate to say that if your effects aren't defined like this then you'll just have bugs
There was a problem hiding this comment.
Bugs you say... I wonder what kind would stem from this?
There was a problem hiding this comment.
Mostly that the effect shouldn't be hidden behind a conditional.
That's part of why your navigation didn't work previously.
|
@Zhuinden thank you for the PR. Great to see how you were able to resolve the issues that were present. |
2 participants
No description provided.